Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/7.0-staging] [mono] Use underlying type in RuntimeHelpers.GetSpanDataFrom #87021

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jun 1, 2023

Backport of #87019 to release/7.0-staging

/cc @lambdageek

Customer Impact

Customers can create ReadOnlySpans from arrays of enum type in their apps targeting mobile and WebAssembly platforms.

Fixes #86865

Testing

CI regression test

Risk

Low: existing code that runs correctly will continue working.

Make it work correctly for spans of enums

Fixes #86865

Note that in net8 RuntimeHelpers.CreateSpan<T> is an intrinsic, so
GetSpanDataFrom is never called directly.

But in net7 CreateSpan is not intrinsified on Mono, so the underlying
method really does get called.
@ghost
Copy link

ghost commented Jun 1, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #87019 to release/7.0-staging

/cc @lambdageek

Customer Impact

Customers can create ReadOnlySpans from arrays of enum type in their apps targeting mobile and WebAssembly platforms.

Testing

CI regression test

Risk

Low

Author: github-actions[bot]
Assignees: -
Labels:

Servicing-consider, area-System.Runtime.CompilerServices, area-VM-meta-mono

Milestone: 7.0.x

@lambdageek
Copy link
Member

Test failure is related. I guess the SDK on net7 doesn't produce exactly the same hidden field for the array data.

@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.7, 7.0.8 Jun 6, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 6, 2023
if (pid == null)
return 3;

var fi = pid.GetField("0B77DC554B4A81403D62BE25FB5404020AD451151D4203D544BF60E3FEDBD8AE", BindingFlags.Static | BindingFlags.NonPublic);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, is this some kind of internal hash? is it stable?

Copy link
Member

@lambdageek lambdageek Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's a hash of the constant value. It's fairly stable - in local testing the same field is generated with the net8.0 and net7.0 SDKs when targeting both the net8.0 and net7.0 runtimes. In CI testing, for some reason the 7.0 branch creates a field with a name that is 1 character longer (ie that final "E" is here on the release/7.0-staging version of the test, but not in the main branch).

I could just blindly take the first matching field in the assembly (and maybe do some minimal checking of its attributes to make sure it's plausibly the correct one), but it seemed more straightforward to just hardcode the name and to return a distinct failure code and a print out of the available fields.

@lambdageek
Copy link
Member

Remaining failures are all known

@lambdageek lambdageek merged commit 079e9e8 into release/7.0-staging Jun 7, 2023
@jkotas jkotas deleted the backport/pr-87019-to-release/7.0-staging branch June 16, 2023 23:39
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants